Check plugin modified state for more lists and improve code quality #3795
Conversation
|
🥷 Code experts: no user but you matched threshold 10 Jack251970 has most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame: To learn more about /:\ gitStream - Visit our Docs ✨ Comment |
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
coderabbitai
bot
added
bug
There was a problem hiding this comment.
Pull Request Overview
This PR refines how plugins are filtered by modified state and reorganizes the PluginManager internals for clarity.
- Switch loops in MainViewModel and Internationalization to use filtered getters that exclude modified plugins
- Refactor
PluginManagerto cache interface-specific plugin lists and replace_modifiedPluginswithModifiedPlugins - Add
GetResultUpdatePluginandGetTranslationPluginsmethods and initialize cached lists inLoadPlugins
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Flow.Launcher/ViewModel/MainViewModel.cs | Use GetResultUpdatePlugin() instead of unfiltered interface call |
| Flow.Launcher.Core/Resource/Internationalization.cs | Replace GetPluginsForInterface<IPluginI18n>() with GetTranslationPlugins() |
| Flow.Launcher.Core/Plugin/PluginManager.cs | Rename and relocate _modifiedPlugins, cache interface lists, add filtered getters |
Comments suppressed due to low confidence (5)
Flow.Launcher.Core/Plugin/PluginManager.cs:40
- [nitpick] Consider renaming
_resultUpdatePluginto_resultUpdatePlugins(plural) to reflect that it holds multiple plugin pairs and maintain consistency with other cached lists.
private static IEnumerable<PluginPair> _resultUpdatePlugin;
Flow.Launcher.Core/Plugin/PluginManager.cs:36
- [nitpick] The private field
ModifiedPluginsuses PascalCase but most private fields in this codebase use a leading underscore (e.g.,_modifiedPlugins). Consider renaming for consistency.
private static readonly List<string> ModifiedPlugins = new();
Flow.Launcher.Core/Plugin/PluginManager.cs:422
- [nitpick] The method
GetResultUpdatePluginreturns multiple entries but uses a singular name. Consider renaming it toGetResultUpdatePluginsto clearly indicate it returns a collection.
public static IList<PluginPair> GetResultUpdatePlugin()
Flow.Launcher.Core/Plugin/PluginManager.cs:427
- Consider adding unit tests for
GetTranslationPluginsandGetResultUpdatePluginto verify that modified plugins are correctly excluded and that filtering behaves as expected.
public static IList<PluginPair> GetTranslationPlugins()
Flow.Launcher.Core/Plugin/PluginManager.cs:185
- Cached plugin lists are only initialized in
LoadPlugins, but not inInitializePluginsAsync. If plugins are reloaded viaInitializePluginsAsync, these caches may become stale. Ensure enumeration is updated in all initialization paths or revert to on-demand retrieval.
_contextMenuPlugins = GetPluginsForInterface<IContextMenu>();
📝 Walkthrough""" WalkthroughThe changes refactor the plugin management system by centralizing and extending how plugin interface collections are handled in the Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant PluginManager
participant PluginPair
App->>PluginManager: LoadPlugins(settings)
PluginManager->>PluginManager: Initialize plugin interface collections (_contextMenuPlugins, _homePlugins, _resultUpdatePlugin, _translationPlugins)
PluginManager->>PluginPair: Filter out modified plugins
App->>PluginManager: GetResultUpdatePlugin() / GetTranslationPlugins()
PluginManager->>App: Return filtered plugin collections
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Flow.Launcher.Core/Plugin/PluginManager.cs (1)
298-298: Inconsistent usage of PluginModified methods.There's an inconsistency in how plugin modification is checked:
- Line 298: Uses
PluginModified(p.Metadata.ID)(internal method)- Line 301: Uses
API.PluginModified(plugin.Metadata.ID)(API method)Based on the retrieved learnings, using the internal
PluginModified()method is preferred for better performance and architectural design.Apply this diff to use the internal method consistently:
- if (API.PluginModified(plugin.Metadata.ID)) + if (PluginModified(plugin.Metadata.ID))Also applies to: 301-301
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Flow.Launcher.Core/Plugin/PluginManager.cs(7 hunks)Flow.Launcher.Core/Resource/Internationalization.cs(2 hunks)Flow.Launcher/ViewModel/MainViewModel.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Flow.Launcher.Core/Resource/Internationalization.cs (2)
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Learnt from: taooceros
PR: Flow-Launcher/Flow.Launcher#2616
File: Flow.Launcher/Flow.Launcher.csproj:7-7
Timestamp: 2024-10-08T15:52:58.573Z
Learning: In the Flow Launcher project, the version number in the `Flow.Launcher.csproj` file is dynamically updated during the CI/CD process.
Flow.Launcher/ViewModel/MainViewModel.cs (2)
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Learnt from: taooceros
PR: Flow-Launcher/Flow.Launcher#3112
File: Flow.Launcher/ViewModel/ResultsViewModel.cs:231-239
Timestamp: 2024-12-07T18:48:14.625Z
Learning: Optimizing the LINQ operations in the method `NewResults` in `ResultsViewModel.cs` did not improve performance.
Flow.Launcher.Core/Plugin/PluginManager.cs (2)
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Learnt from: jjw24
PR: Flow-Launcher/Flow.Launcher#2448
File: Plugins/Flow.Launcher.Plugin.Sys/ThemeSelector.cs:16-20
Timestamp: 2025-01-18T10:10:18.414Z
Learning: In Flow Launcher's plugin system, the PluginInitContext parameter passed to plugin constructors is guaranteed to be non-null by the plugin initialization system, making null checks unnecessary.
🧬 Code Graph Analysis (2)
Flow.Launcher.Core/Resource/Internationalization.cs (1)
Flow.Launcher.Core/Plugin/PluginManager.cs (2)
PluginManager(23-729)PluginManager(162-168)
Flow.Launcher.Core/Plugin/PluginManager.cs (4)
Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (5)
List(177-177)List(445-445)T(319-319)PluginModified(539-539)InstallPlugin(561-561)Flow.Launcher.Core/Plugin/PluginsLoader.cs (5)
List(25-56)IEnumerable(58-137)IEnumerable(139-151)IEnumerable(153-165)PluginsLoader(17-166)Flow.Launcher.Infrastructure/UserSettings/PluginSettings.cs (4)
UpdatePluginSettings(41-90)Plugin(92-99)Plugin(101-105)Plugin(108-134)Flow.Launcher.Plugin/PluginMetadata.cs (1)
PluginMetadata(10-163)
🪛 GitHub Check: Check Spelling
Flow.Launcher.Core/Plugin/PluginManager.cs
[warning] 182-182:
metadatas is not a recognized word. (unrecognized-spelling)
[warning] 180-180:
metadatas is not a recognized word. (unrecognized-spelling)
[warning] 179-179:
metadatas is not a recognized word. (unrecognized-spelling)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (12)
Flow.Launcher.Core/Resource/Internationalization.cs (3)
77-77: Excellent refactoring to use centralized plugin management.The change from
GetPluginsForInterface<IPluginI18n>()toGetTranslationPlugins()aligns perfectly with the PR objectives of extending plugin modified state checking. The new method automatically filters out modified plugins, improving code quality and consistency.
281-281: Good documentation improvement.The added comment clearly explains the purpose of the loop, enhancing code readability and maintainability.
282-282: Consistent application of the new plugin management approach.This change mirrors the improvement at line 77, ensuring consistent use of the centralized
GetTranslationPlugins()method throughout the internationalization class.Flow.Launcher/ViewModel/MainViewModel.cs (1)
263-263: Proper implementation of centralized plugin management for result updates.The change from
GetPluginsForInterface<IResultUpdated>()toGetResultUpdatePlugin()successfully extends the plugin modified state checking to the result update functionality, directly addressing the PR objectives. This ensures modified plugins are excluded from result update events.Flow.Launcher.Core/Plugin/PluginManager.cs (8)
36-36: Good refactoring: Improved accessibility and naming consistency.The rename from
_modifiedPluginstoModifiedPluginsand changing from private to readonly improves consistency with other public collections in the class likeAllPluginsandGlobalPlugins.
38-41: Well-structured addition of plugin interface collections.The new private static fields for
_resultUpdatePluginand_translationPluginsfollow the established pattern of the existing_contextMenuPluginsand_homePluginscollections. This centralization will improve performance by avoiding repeated interface filtering operations.
177-189: Excellent code organization improvement.The changes to
LoadPluginsmethod improve code quality by:
- Storing
metadatasin a local variable for better readability- Centralizing plugin collection initialization after all plugins are loaded
- Using the generic
GetPluginsForInterface<T>()method consistentlyThis ensures all interface collections are properly initialized and available for the new filtering methods.
416-420: Good encapsulation by making the method private.Making
GetPluginsForInterface<T>()private is appropriate since it's now used internally for initialization, and the new public methods provide the proper filtered access.
422-430: Excellent addition of filtered plugin accessor methods.The new
GetResultUpdatePlugin()andGetTranslationPlugins()methods provide clean, filtered access to plugin collections while excluding modified plugins. This approach is consistent with the existingValidPluginsForHomeQuery()method and aligns with the PR's objective of extending modified state checking.
435-435: Improved plugin filtering consistency.The update to filter out modified plugins in
GetContextMenusForPlugin()is excellent and aligns with the PR's objective. The use of the directPluginModified()method is also consistent with the architectural preference noted in the retrieved learnings.
463-463: Consistent application of modified plugin filtering.The filtering logic in
IsHomePlugin()correctly excludes modified plugins, maintaining consistency with other plugin collection methods in the class.
561-561: Consistent field rename implementation.All references to the renamed
ModifiedPluginsfield have been properly updated throughout the class, maintaining consistency and correctness.Also applies to: 568-568, 655-655, 724-724
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
Flow.Launcher.Core/Plugin/PluginManager.cs (1)
297-303: Use internalPluginModifiedinstead ofAPI.PluginModifiedPer prior guidance (PR #3791), when inside
PluginManagerprefer the internal staticPluginModified(id)helper for performance and decoupling. Replace the external API call here.-if (API.PluginModified(plugin.Metadata.ID)) +if (PluginModified(plugin.Metadata.ID))
♻️ Duplicate comments (2)
Flow.Launcher.Core/Plugin/PluginManager.cs (2)
427-430: Same comments as above apply toGetTranslationPlugins– plural naming and potential caching ofToList()result.
561-569: Duplicate additions to ModifiedPlugins
ModifiedPlugins.Add(id)is called in three different code paths. WithConcurrentBagthis can add the same ID many times, inflating memory and slowing.Contains. Switching to the set-style structure suggested earlier removes that concern.Also applies to: 655-657, 724-725
🧹 Nitpick comments (2)
Flow.Launcher.Core/Plugin/PluginManager.cs (2)
38-41: Plural-naming nitpick
_resultUpdatePluginshould be_resultUpdatePlugins(plural) for consistency with the other collections and to avoid cognitive friction.
422-425: Method name & return type
- Method should be plural:
GetResultUpdatePlugins()to match its siblings.- You rebuild the list on every call (
ToList()). If the modified-filter doesn’t change often, cache the list or returnIEnumerableto avoid allocations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher.Core/Plugin/PluginManager.cs(7 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Flow.Launcher.Core/Plugin/PluginManager.cs (2)
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Learnt from: jjw24
PR: Flow-Launcher/Flow.Launcher#2448
File: Plugins/Flow.Launcher.Plugin.Sys/ThemeSelector.cs:16-20
Timestamp: 2025-01-18T10:10:18.414Z
Learning: In Flow Launcher's plugin system, the PluginInitContext parameter passed to plugin constructors is guaranteed to be non-null by the plugin initialization system, making null checks unnecessary.
🪛 GitHub Check: Check Spelling
Flow.Launcher.Core/Plugin/PluginManager.cs
[warning] 182-182:
metadatas is not a recognized word. (unrecognized-spelling)
[warning] 180-180:
metadatas is not a recognized word. (unrecognized-spelling)
[warning] 179-179:
metadatas is not a recognized word. (unrecognized-spelling)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (1)
Flow.Launcher.Core/Plugin/PluginManager.cs (1)
185-188: Order-of-initialisation subtletyThese interface-specific collections are initialised in
LoadPlugins. Ensure that every public method that touches them (e.g.ValidPluginsForHomeQuery,GetContextMenusForPlugin, etc.) is guaranteed not to be called beforeLoadPluginshas finished; otherwiseNullReferenceExceptionwill surface.
If that guarantee doesn’t already exist, guard the fields or move the initialisation into the static ctor.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Flow.Launcher.Core/Plugin/PluginManager.cs (1)
35-36: Thread-safe collection adopted as discussed.The change to
ConcurrentBag<string>addresses the concurrent access issue mentioned in the PR objectives.
🧹 Nitpick comments (2)
Flow.Launcher/ViewModel/MainViewModel.cs (1)
276-276: Consider renaming toGetResultUpdatePlugins()for consistency.The method returns an
IList<PluginPair>(a collection) but has a singular name. This is inconsistent withGetTranslationPlugins()which correctly uses plural form.Flow.Launcher.Core/Plugin/PluginManager.cs (1)
298-298: Consider future optimization for modified plugin lookups.The current implementation performs O(n) lookups in
ConcurrentBagfor eachPluginModifiedcheck. While acceptable for typical usage (few modified plugins), consider usingConcurrentDictionary<string, byte>if performance becomes an issue with many concurrent plugin updates.Also applies to: 314-314, 424-424, 429-429, 435-435, 463-463
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Flow.Launcher.Core/Plugin/PluginManager.cs(7 hunks)Flow.Launcher.Core/Resource/Internationalization.cs(2 hunks)Flow.Launcher/ViewModel/MainViewModel.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Flow.Launcher.Core/Resource/Internationalization.cs (2)
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Learnt from: taooceros
PR: Flow-Launcher/Flow.Launcher#2616
File: Flow.Launcher/Flow.Launcher.csproj:7-7
Timestamp: 2024-10-08T15:52:58.573Z
Learning: In the Flow Launcher project, the version number in the `Flow.Launcher.csproj` file is dynamically updated during the CI/CD process.
Flow.Launcher/ViewModel/MainViewModel.cs (1)
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Flow.Launcher.Core/Plugin/PluginManager.cs (4)
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Learnt from: jjw24
PR: Flow-Launcher/Flow.Launcher#2448
File: Plugins/Flow.Launcher.Plugin.Sys/ThemeSelector.cs:16-20
Timestamp: 2025-01-18T10:10:18.414Z
Learning: In Flow Launcher's plugin system, the PluginInitContext parameter passed to plugin constructors is guaranteed to be non-null by the plugin initialization system, making null checks unnecessary.
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3500
File: Flow.Launcher/Storage/TopMostRecord.cs:145-149
Timestamp: 2025-05-01T05:38:25.673Z
Learning: For the MultipleTopMostRecord implementation in Flow.Launcher, sequence order of records in the ConcurrentBag does not need to be preserved, as confirmed by the developer. The unordered nature of ConcurrentBag is acceptable for this implementation.
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3500
File: Flow.Launcher/Storage/TopMostRecord.cs:191-197
Timestamp: 2025-05-01T05:35:21.852Z
Learning: ConcurrentBag.TryTake(out T) in C# removes an arbitrary item from the bag, not necessarily the one matching the out parameter. This can lead to removing the wrong element when trying to remove a specific item. A safer approach is to reconstruct the collection using filtering or use an ordered collection with proper removal methods.
🪛 GitHub Check: Check Spelling
Flow.Launcher.Core/Plugin/PluginManager.cs
[warning] 182-182:
metadatas is not a recognized word. (unrecognized-spelling)
[warning] 180-180:
metadatas is not a recognized word. (unrecognized-spelling)
[warning] 179-179:
metadatas is not a recognized word. (unrecognized-spelling)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (3)
Flow.Launcher.Core/Resource/Internationalization.cs (1)
77-77: LGTM!The changes correctly use the new
GetTranslationPlugins()method instead of the now-privateGetPluginsForInterface<IPluginI18n>(). The added comment improves code clarity.Also applies to: 281-282
Flow.Launcher.Core/Plugin/PluginManager.cs (2)
38-42: Well-structured initialization of plugin interface collections.The collections are properly initialized after all plugins are loaded, ensuring complete interface discovery. The null-coalescing operator in
GetPluginsForInterface(line 419) provides safety if accessed before initialization.Also applies to: 184-188
422-430: Correct implementation of filtered plugin accessors.Both methods properly filter out modified plugins before returning the collections, maintaining consistency with the existing plugin management design.
…_modified Check plugin modified state for more lists and improve code quality
Check plugin modified state for more lists and improve code quality
2 participants
Follow on with #3791.